Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.44%. Comparing base (292e181) to head (4867a3b).
Report is 289 commits behind head on main.

@@            Coverage Diff             @@
##             main     #371      +/-   ##
==========================================
+ Coverage   77.66%   78.44%   +0.78%     
==========================================
  Files          27       27              
  Lines        3626     3758     +132     
==========================================
+ Hits         2816     2948     +132     
  Misses        810      810              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Note: the linting failure is because ruff changed its rules -- it is not due to a code change. I'm planning to ignore it for now and open an issue to fix it later.

This might be better as a new issue than as something to do for this PR to get merged...

I used the review widget, I noticed that while we got an indication the whole model was bad, there was no way to find out WHY the model was bad. I purposely mis-entered the units for one entry and there was no clear indication that was the problem.

From what I understand of ipyautoui, we are suppressing the actual validation error message (which makes sense). But would it be possible to have a way of displaying the validation error if the user so chooses? Or if you want to get clever you could parse the error message and visually flag the individual bad entries, although that seems like a lot more hard work.

All the following tests were done in Jupyter Lab, I did test a little with VS Code and it seems consistent except some of the drop downs do NOT render properly. That might be an issue with how iPyAutoui creates its custom widgets...

Other glitches I noticed:

• The dropdown menus don't seem to work in expected ways sometimes: Specifically:

  • Case 1: Repeated options

    1. Run all cells
    2. Select make a new camera (I used ` from drop down).
    3. Select adu for data units and 1.5 electrons / adu for gain
    4. Glitch? Read noise now presents three identical copies of 10.0 electron for noise.
    5. Glitch? Dark current now presents three identical copies of 0.01 electron / sec for noise.
    6. Glitch? I was presented with the option to select various DN, including two with the wrong, invalid units given all the previous settings. Not necessarily easily avoidable, but I could imagine it as a bit confusing for first-timers at photometry.

  • Case 2: No Option

    1. Run all cells
    2. Select to edit the Testing Camera
    3. Go to the dropdown menu for Gain... it has no options. Same with Max Data Value.

• Another issue: AAVSO filters appear twice in dropdown

  1. I opted to edit the built in Filter wheel 1 under the Passband map tab.
  2. I clicked the red + button
  3. I added 'r' as a hypothetical filter and selected the dropdown... the R and I filters were listed twice

Reviewing this is on my to-do list for tonight. Just FYI @mwcraig

Case 1: Repeated options ....
Glitch? Read noise now presents three identical copies of 10.0 electron for noise.

Can you double check? The last option is still 10.0 photon for me.

Glitch? Dark current now presents three identical copies of 0.01 electron / sec for noise.

Same here, I'm seeing 0.01 photon / sec as the last option.

Glitch? I was presented with the option to select various DN, including two with the wrong, invalid units given all the previous settings. Not necessarily easily avoidable, but I could imagine it as a bit confusing for first-timers at photometry.

More a design feature, though maybe it ends up being confusing. Each dropdown has three options. The set of first options are consistent with each other, the set of second options are consistent and the set of third options are consistent. However, there is no updating of the suggestions as fields are filled out. That could maybe be added iun the future...

Case 2: No Option

Run all cells
Select to edit the Testing Camera
Go to the dropdown menu for Gain... it has no options. Same with Max Data Value.

This is because by default ipyautoui makes an ipywidgets.Combobox which is a combination text box and dropdown. The dropdowns are only suggestions; any value is allowed.

In addition, the dropdowns are only shown if there is no text in the box.

You can see the same behavior in the ipywidgets documentation I linked to above.

Another issue: AAVSO filters appear twice in dropdown
...
I added 'r' as a hypothetical filter and selected the dropdown... the R and I filters were listed twice

Ah, that is because the AAVSO effectively has its own passsband map which maps R and Rc to R and similar for I, so each shows up twice.

Will open a separate issue for this.

This might be better as a new issue than as something to do for this PR to get merged...

I used the review widget, I noticed that while we got an indication the whole model was bad, there was no way to find out WHY the model was bad. I purposely mis-entered the units for one entry and there was no clear indication that was the problem.

From what I understand of ipyautoui, we are suppressing the actual validation error message (which makes sense). But would it be possible to have a way of displaying the validation error if the user so chooses?

This is essentially the idea in: maxfordham/ipyautoui#308

For now, can you open an issue in this repo, @JuanCab, to add an option to display the errors, maybe by clicking on the red x?

Case 1: Repeated options ....
Glitch? Read noise now presents three identical copies of 10.0 electron for noise.

Can you double check? The last option is still 10.0 photon for me.

Glitch? Dark current now presents three identical copies of 0.01 electron / sec for noise.

Same here, I'm seeing 0.01 photon / sec as the last option.

Here are screen shots showing the menus I am seeing:

Here are screen shots showing the menus I am seeing:

Ooooohhhhh, for a moment I was an old man shaking his fist in the sky in frustration that something was broken, but now I think I know what is going on.

What I said before was not quite right:

In addition, the dropdowns are only shown if there is no text in the [combo] box.

What is displayed in the combobox is only the options consistent with the text entered so far, so when 10.0 electron is already in the read noise, the combobox only shows the two entries consistent with that. If you were to erase the contents of that box you would see three choices, the last of which would be 10.0 photon.

I'm not sure what to do about this....

I am starting to review this and just had a quick question. Do we need to provide an input box for the focal length of the system being used? I know we have our FL in the FITS header but does the "Sky" coordinate system, which I assume is WCS, need a focal length to plate solve the images?

. Do we need to provide an input box for the focal length of the system being used? I know we have our FL in the FITS header but does the "Sky" coordinate system, which I assume is WCS, need a focal length to plate solve the images?

I don't think so -- the FL can be calculated from some of the parameters that the plate solve generates. I think it could also be used as an input, but we haven't done it that way.

I gather, though, that that is something you need to provide as an input for your plate solves?

Yeah, from my experience using Pixinsight and ASTAP (standalone platesolver) they need a focal length. It doenst need to be exact but as long as it is within putting distance of the actual FL it works.

I think we don't need that here because I've been assuming images are already plate solved....

Looks good, two typo suggestions made, but otherwise, good to go.

Merging once tests pass 😬 🎉